Skip to content

CLI: Add VT Support library and update tests/callbacks to use it#40710

Open
dkbennett wants to merge 14 commits into
masterfrom
user/dkbennett/vt2
Open

CLI: Add VT Support library and update tests/callbacks to use it#40710
dkbennett wants to merge 14 commits into
masterfrom
user/dkbennett/vt2

Conversation

@dkbennett

Copy link
Copy Markdown
Member

Summary of the Pull Request

This is a foundational addition of VT support classes, wrappers, and constants to make working with VT sequences consistent and simpler without having to deal with the actual sequences and restoration. It removes the old ChangeTerminalMode.h and rolls that into the new VTSupport.h and cpp. This is added under windows/common so it can be utilized by more than just the CLI, but the unit tests are in with the CLI.

This should also help alleviate some other pulls where VT sequences are being defined in multiple locations, and also serves as a foundation for enhanced VT support in the CLI.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

  • Add VT support classes to windows common
  • Add unit tests for the VT classes and sequences.
  • Update tests to use the new common VT support code.
  • Update the BuildImageCallback and ImageProgressCallback to use the VT support classes instead of directly defined sequences.

Validation Steps Performed

  • Ran E2E tests, the Interactive TTY, Image Build and Pull tests to ensure they work as expected.
  • Ran the new unit tests.
  • Manual testing of the image build and image pull commands to verify the callback output is as expected.

Copilot AI review requested due to automatic review settings June 4, 2026 11:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a shared VT (virtual terminal) support library under src/windows/common to centralize VT sequence constants/construction and console-mode RAII helpers, then updates WSLC callbacks and tests to consume the shared implementation (removing the prior WSLC-local ChangeTerminalMode.h).

Changes:

  • Added wsl::windows::common::vt VT sequence helpers, stream/concat operators, and console-mode RAII helpers (VTSupport.h/.cpp) and wired them into the common build.
  • Migrated WSLC image progress/build callbacks away from WSLC-local VT definitions to the shared VT support.
  • Updated WSLC E2E helpers and added a new unit test suite validating VT sequences and console-mode helpers.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
test/windows/wslc/WSLCCLIVTSupportUnitTests.cpp Adds new unit tests covering VT sequences and console-mode RAII helpers.
test/windows/wslc/e2e/WSLCExecutor.h Adds Sequence overloads for stdout/stderr expectations in interactive sessions.
test/windows/wslc/e2e/WSLCExecutor.cpp Adjusts formatting/expectations to use Sequence-based constants.
test/windows/wslc/e2e/WSLCE2EHelpers.h Replaces ad-hoc VT macros with shared VTSupport sequences and helpers.
src/windows/wslc/services/ImageProgressCallback.h Switches includes and member types to wsl::windows::common::vt helpers.
src/windows/wslc/services/ImageProgressCallback.cpp Uses shared cursor-movement sequences instead of raw escape literals.
src/windows/wslc/services/ChangeTerminalMode.h Removes WSLC-local cursor/VT RAII helpers (superseded by common VTSupport).
src/windows/wslc/services/BuildImageCallback.h Switches to shared VT enablement helper; minor type adjustments for line counts.
src/windows/wslc/services/BuildImageCallback.cpp Replaces raw escape formatting with shared VT sequences and stream composition.
src/windows/common/VTSupport.h Adds new public VT support API (sequences, operators, and RAII helpers).
src/windows/common/VTSupport.cpp Implements VT constants/builders and DA1 parsing.
src/windows/common/CMakeLists.txt Adds VTSupport sources/headers to the windows/common build.

Comment thread src/windows/common/VTSupport.h
Comment thread src/windows/common/VTSupport.h Outdated
Comment thread src/windows/common/VTSupport.h Outdated
Comment thread src/windows/common/VTSupport.cpp Outdated
Comment thread src/windows/common/VTSupport.cpp Outdated
Comment thread src/windows/common/VTSupport.cpp Outdated
Copilot AI review requested due to automatic review settings June 4, 2026 12:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Comment thread src/windows/common/VTSupport.h
Comment thread src/windows/common/VTSupport.h Outdated
Comment thread src/windows/common/VTSupport.cpp Outdated
Comment thread src/windows/wslc/services/ImageProgressCallback.cpp
Comment thread src/windows/common/VTSupport.cpp Outdated
Copilot AI review requested due to automatic review settings June 4, 2026 13:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Comment thread test/windows/wslc/e2e/WSLCE2EHelpers.h
Comment thread src/windows/wslc/services/ImageProgressCallback.h
Comment thread src/windows/common/VTSupport.h
Comment thread src/windows/common/VTSupport.cpp
Copilot AI review requested due to automatic review settings June 4, 2026 20:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Comment thread test/windows/wslc/WSLCCLIVTSupportUnitTests.cpp
Comment thread src/windows/wslc/services/ImageProgressCallback.cpp
Comment thread src/windows/common/VTSupport.h
Comment thread src/windows/common/VTSupport.cpp Outdated
Copilot AI review requested due to automatic review settings June 5, 2026 20:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Comment thread src/windows/common/VTSupport.h Outdated
Comment thread src/windows/wslc/services/BuildImageCallback.cpp Outdated
@dkbennett dkbennett marked this pull request as ready for review June 6, 2026 00:00
@dkbennett dkbennett requested a review from a team as a code owner June 6, 2026 00:00
Copilot AI review requested due to automatic review settings June 6, 2026 00:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Comment thread src/windows/wslc/services/ImageProgressCallback.h
Copilot AI review requested due to automatic review settings June 8, 2026 10:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.

Comment thread src/windows/common/VTSupport.h
Comment thread src/windows/common/VTSupport.h
Comment thread src/windows/common/VTSupport.cpp Outdated

@OneBlue OneBlue left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think there's a few things that we simplify, but this design looks great !


ChangeTerminalMode(HANDLE console, bool cursorVisible) : m_console(console)
{
if (!wsl::windows::common::wslutil::IsConsoleHandle(console))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since we have a .cpp file, I would have a preference for moving the implementation of these methods to the .cpp, so we don't need to re-compile them every time they're included


// operator<< for stream output.
// Widens the narrow sequence bytes (all ASCII) into a wide string for wostream output.
inline std::wostream& operator<<(std::wostream& o, const Sequence& s)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the objective of those is to use std::wcout, but for our usecase I would recommend to keep using the WriteConsoleW() mecanism since it's significantly faster.

using namespace wsl::windows::common::vt;

auto ImageProgressCallback::MoveToLine(SHORT line)
void ImageProgressCallback::WriteTerminal(std::wstring_view content) const

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we're already defining this method in BuildImageCallback, I'd recommend factoring this method and moving it out to wslutil .

At some point, I think WSL should also move to it, but let's do that in a future change


// operator<< for stream output.
// Widens the narrow sequence bytes (all ASCII) into a wide string for wostream output.
inline std::wostream& operator<<(std::wostream& o, const Sequence& s)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that this was originally designed to use std::wcout to actually write to the console, but in wslc's case, I would recommend not using std::wcout, but directly calling into WriteTerminal.

Maybe we could simplify a bit by directly using Get() when we print the sequence to the terminal (and we could also use this in the tests).

That would allow us to get rid of operator<< + string / wstring methods (our version of std::format() automatically handles string <-> wstring conversions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants